-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds an output to docs files #33
Conversation
@gssbzn, could you take a look at this PR when you have time and let me know if the Go code looks ok? Thank you both!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
general lgtm with some minor go recommendations, this looks like a cool addition
output.go
Outdated
removerange := strings.ReplaceAll(cmd.Annotations["output"], "{{range .Results}}", "") | ||
removeend := strings.ReplaceAll(removerange, "{{end}}", "") | ||
bracketsremoved1 := strings.ReplaceAll(removeend, "{{.", "<") | ||
bracketsremoved2 := strings.ReplaceAll(bracketsremoved1, "}}", ">") | ||
replacevariable := strings.ReplaceAll(bracketsremoved2, "%s", "<Name>") | ||
replacevariable2 := strings.ReplaceAll(replacevariable, "\n", "\n ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[q] can we use the same string? like
removerange := strings.ReplaceAll(cmd.Annotations["output"], "{{range .Results}}", "") | |
removeend := strings.ReplaceAll(removerange, "{{end}}", "") | |
bracketsremoved1 := strings.ReplaceAll(removeend, "{{.", "<") | |
bracketsremoved2 := strings.ReplaceAll(bracketsremoved1, "}}", ">") | |
replacevariable := strings.ReplaceAll(bracketsremoved2, "%s", "<Name>") | |
replacevariable2 := strings.ReplaceAll(replacevariable, "\n", "\n ") | |
output := strings.ReplaceAll(cmd.Annotations["output"], "{{range .Results}}", "") | |
output = strings.ReplaceAll(output, "{{end}}", "") | |
output = strings.ReplaceAll(output, "{{.", "<") | |
output = strings.ReplaceAll(output, "}}", ">") | |
output = strings.ReplaceAll(output, "%s", "<Name>") | |
output = strings.ReplaceAll(output, "\n", "\n ") |
also not sure if this is the best option from the top of my head to do multi string replacement on go
output.go
Outdated
w := new(tabwriter.Writer) | ||
w.Init(buf, 0, 0, 0, ' ', tabwriter.Debug|tabwriter.AlignRight) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this matters but these are the values we use for the tabwritter on the cli
https://github.com/mongodb/mongodb-atlas-cli/blob/7a304c4870c026d80e5b26391efad0e2c48f0f26/internal/templatewriter/templatewriter.go#L23-L34
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I experimented with these a bit and made some changes to the code overall to reflect a different alignment and padding for this. The thing that has me stumped right now: when I add 3 spaces to the beginning of the first line for these, it messes up the alignment of all other headers by 3 spaces. But without the 3 spaces it won't be in the code box.
With 3 extra spaces at the beginning: ID aligns correctly, others are 3 spaces too far in:
Without 3 extra spaces at the beginning: All except ID align correctly:
output.go
Outdated
.. code-block:: | ||
|
||
`) | ||
if strings.HasSuffix(cmd.Annotations["output"], "}"+"\n") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if strings.HasSuffix(cmd.Annotations["output"], "}"+"\n") { | |
if strings.HasSuffix(cmd.Annotations["output"], "}\n") { |
output.go
Outdated
|
||
buf.WriteString(outputHeader) | ||
buf.WriteString(` | ||
If the command succeeds, the CLI prints a message similar to the following and replaces the values in brackets with your values: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I know this is definitely a nit.. For some reason to me "prints a message" sounds like the returns are informational instead of possible data/actions. The CLI also isn't really doing a replace, we're just trying to represent placeholders. I've made a suggestion but I'm not sure its that much better, up to you.
If the command succeeds, the CLI prints a message similar to the following and replaces the values in brackets with your values: | |
If the command succeeds, the CLI returns output similar to the following sample. Values in brackets represent your values. |
@gssbzn @melissamahoney-mongodb Ready for another look! I made both of your suggested changes and I fixed the formatting on the responses with columns. Updated screenshots of the results are in the PR description. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh love that you got the tabbed output to align! This is such an awesome addition, thanks Sarah!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing addition here
Thank you!! @gssbzn looks like I don't have permission to merge on this repo, please merge when you have time. |
Proposed changes
This experimental PR adds an output section to the docs files. It would depend on an "output" annotation, which would use the template already defined in each file (listTemplate, describeTemplate, etc) that reflects the output in the CLI itself.
I tested this locally and I am including screenshots of create, delete, list, and describe commands after a local test with these changes. I also included a screenshot of what the annotation looks like.
Additionally, this PR removes the work from my last PR that added a line for requiredRole because we implemented that as part of the long descriptions instead (just to prevent use of this annotation by mistake)
Jira ticket:
https://jira.mongodb.org/browse/DOCSP-26859
Checklist
make fmt
and formatted my code